Conversation
| { | ||
| "name": "react-spreadsheet", | ||
| "version": "0.8.4", | ||
| "name": "@silicon-jungle/fork-react-spreadsheet", |
There was a problem hiding this comment.
This is probably a mistake. Must be "react-spreadsheet".
There was a problem hiding this comment.
Yeah, this should not be included. I was just running this as a test. Not sure why I pushed this up. Will fix.
| "rollup-plugin-dts": "^5.3.0", | ||
| "rollup-plugin-postcss": "^4.0.1", | ||
| "rollup-plugin-typescript2": "^0.30.0", | ||
| "rollup-plugin-typescript2": "^0.35.0", |
There was a problem hiding this comment.
Not sure how version bumps are handled in this package, but they're probably not needed for this feature. I'd revert this as well.
There was a problem hiding this comment.
Woops - yeah, this was accidental.
| onChange?: (data: Matrix.Matrix<CellType>) => void; | ||
| onChange?: ( | ||
| data: Matrix.Matrix<CellType>, | ||
| evaluatedData: Matrix.Matrix<CellType> |
There was a problem hiding this comment.
I like this change! Makes it easy to get exactly what you need.
When the API grows, it might be better to expose a single Matrix where each cell has properties like raw and evaluated along with other data in it. But that's a bit of a higher-level-decision.
There was a problem hiding this comment.
Yeah, I feel like merging the two data structured together would be better. Should we merge in something separate like this and merge them in together at a later date? Or should we talk about a higher-level change here?
There was a problem hiding this comment.
For now I think merging them won't be necessary. It will introduce an additional performance cost that I don't think is justified.
| // Check onChange is called | ||
| expect(EXAMPLE_PROPS.onChange).toBeCalledTimes(1); | ||
| expect(EXAMPLE_PROPS.onChange).toBeCalledWith(EXAMPLE_MODIFIED_DATA); | ||
| // expect(EXAMPLE_PROPS.onChange).toBeCalledWith(EXAMPLE_MODIFIED_DATA); |
There was a problem hiding this comment.
As you said, a test would be nice. Need some help? :)
| // Handlers | ||
| /** Callback called on key down inside the spreadsheet. */ | ||
| onKeyDown?: (event: React.KeyboardEvent) => void; | ||
| /** Callback called when the Spreadsheet's data changes. */ |
There was a problem hiding this comment.
Would love an updated doc for this prop
| find-up "^5.0.0" | ||
| html-entities "^2.1.0" | ||
| loader-utils "^2.0.4" | ||
| schema-utils "^3.0.0" |
There was a problem hiding this comment.
As @Carnageous correctly pointed out, I expect the yarn.lock to not change in this PR.
|
Great work @siliconjungle and thank you @Carnageous for helping with the review. I would love to merge once the fixes are in place. |
Pull Request Test Coverage Report for Build 6271723049
💛 - Coveralls |
4 participants
Haven't added the test yet - wanted to check whether this was the recommended way to make the suggested change?